Add context in name validation errors by djc · Pull Request #301 · rustls/webpki

djc · 2024-12-22T16:40:03Z

This pretty much achieves my goals and is probably workable but not quite pretty:

No Copy for Error means a semver-incompatible change
- Semver-incompatible changes in webpki specifically are relatively manageable since most users don't depend on it directly
A little strange to get different errors for alloc vs no alloc?

I can fix test failures etc if we think this is worth doing.

ctz · 2024-12-23T09:32:07Z

I think this is definitely worth doing. In the future we could also do similar treatment to:

CertExpired
CertNotValidYet
CrlExpired

(with the current and cert times)

The other future improvement would be to have a more detailed fmt::Display, which could say, perhaps:

certificate is not valid for "example.com".
instead, the certificate contained "*.example.com", "foobar.org".

codecov · 2024-12-23T21:18:09Z

Codecov Report

Attention: Patch coverage is 97.67442% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.30%. Comparing base (5dc0926) to head (44fc4a6).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/subject_name/dns_name.rs	94.28%	2 Missing ⚠️
src/error.rs	0.00%	1 Missing ⚠️
src/subject_name/ip_address.rs	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   97.23%   97.30%   +0.07%     
==========================================
  Files          20       20              
  Lines        4225     4343     +118     
==========================================
+ Hits         4108     4226     +118     
  Misses        117      117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djc · 2024-12-23T21:18:35Z

I think this is definitely worth doing. In the future we could also do similar treatment to:

CertExpired

CertNotValidYet

CrlExpired

(with the current and cert times)

The other future improvement would be to have a more detailed fmt::Display, which could say, perhaps:
certificate is not valid for "example.com".
instead, the certificate contained "*.example.com", "foobar.org".

Good ideas! Have not implemented them yet, figure it makes sense to get this in first as a proof point and to iron out the pattern here.

src/error.rs

src/subject_name/verify.rs

cpu

Here's some initial feedback. Overall I think it's 👍

src/der.rs

src/error.rs

src/subject_name/verify.rs

djc · 2025-01-06T13:30:17Z

Fixed the test failures and tacked on some refactoring.

cpu

The error context changes LGTM.

I'm a little bit nervous about the NameIterator simplification at the end w.r.t the directory_name bool change. I think it's probably OK but want to review that part of the diff with fresh eyes before I +1 the whole PR. I find the pre-existing code hard to reason about when I haven't been working in this repo recently.

If you want to pull refactoring out into a second PR to land the error improvement sooner that's OK with me too.

src/error.rs

src/subject_name/verify.rs

cpu · 2025-01-14T22:25:59Z

To see the specific tasks where the Asana app for GitHub is being used, see below:
https://app.asana.com/0/0/1209099649554591
https://app.asana.com/0/0/1209099649554598

Is this something you're using to track your F/OSS work? FWIW the links are 404s for me as an unauthed user so I'm not sure it's useful for Asana to be putting them in the PR desc.

djc · 2025-01-15T07:56:49Z

Is this something you're using to track your F/OSS work? FWIW the links are 404s for me as an unauthed user so I'm not sure it's useful for Asana to be putting them in the PR desc.

Yeah, trying out Asana to track my personal tasks and its GitHub integration turned out to be more bidirectional than I thought. Will remove it.

djc · 2025-01-15T07:57:29Z

If you want to pull refactoring out into a second PR to land the error improvement sooner that's OK with me too.

That sounds like a good idea!

djc · 2025-01-15T14:30:19Z

I'm a little bit nervous about the NameIterator simplification at the end w.r.t the directory_name bool change.

It's gone for now, will bring it back in a separate PR after this is merged.

cpu

Nice :-)

djc · 2025-01-16T10:40:04Z

Will wait to see if @ctz has any more detailed feedback.

ctz

(beach review: lgtm!)

djc requested review from cpu and ctz December 22, 2024 16:40

djc force-pushed the unexpected-name branch from c821e9b to ab17a75 Compare December 22, 2024 16:42

djc force-pushed the unexpected-name branch from ab17a75 to 45bc910 Compare December 23, 2024 21:16

djc marked this pull request as ready for review December 23, 2024 21:16

djc force-pushed the unexpected-name branch 4 times, most recently from 7bc8a7a to 98d5332 Compare December 23, 2024 21:55

djc commented Dec 23, 2024

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/subject_name/verify.rs Show resolved Hide resolved

djc force-pushed the unexpected-name branch 2 times, most recently from a40d2ca to c67bd76 Compare December 24, 2024 13:06

cpu reviewed Dec 24, 2024

View reviewed changes

src/der.rs Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/subject_name/verify.rs Show resolved Hide resolved

djc force-pushed the unexpected-name branch from c67bd76 to 56700b4 Compare January 6, 2025 09:20

djc added 2 commits January 6, 2025 10:23

Remove Copy impl from Error

398fb83

Pass Cert directly to name verification functions

fd892e7

djc force-pushed the unexpected-name branch 10 times, most recently from 18dc1a0 to 1757dc0 Compare January 6, 2025 13:12

cpu reviewed Jan 14, 2025

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

src/subject_name/verify.rs Show resolved Hide resolved

src/subject_name/verify.rs Outdated Show resolved Hide resolved

djc force-pushed the unexpected-name branch from 1757dc0 to d384bac Compare January 15, 2025 14:28

cpu approved these changes Jan 15, 2025

View reviewed changes

Add context in name validation errors

44fc4a6

djc force-pushed the unexpected-name branch from d384bac to 44fc4a6 Compare January 16, 2025 10:25

ctz approved these changes Jan 16, 2025

View reviewed changes

djc added this pull request to the merge queue Jan 16, 2025

Merged via the queue into main with commit c32bfb9 Jan 16, 2025
58 checks passed

djc deleted the unexpected-name branch January 16, 2025 13:24

cpu added the breaking-change label Jan 19, 2025

ctz mentioned this pull request Jan 28, 2025

Add details to time-based errors #315

Merged

Conversation

djc commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctz commented Dec 23, 2024

Uh oh!

codecov bot commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djc commented Dec 23, 2024

Uh oh!

Uh oh!

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc commented Jan 6, 2025

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpu commented Jan 14, 2025

Uh oh!

djc commented Jan 15, 2025

Uh oh!

djc commented Jan 15, 2025

Uh oh!

djc commented Jan 15, 2025

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

djc commented Jan 16, 2025

Uh oh!

ctz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

djc commented Dec 22, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading